Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move tests from doctest to Catch2 #301

Merged
merged 36 commits into from
Dec 18, 2024

Conversation

Gulin7
Copy link
Contributor

@Gulin7 Gulin7 commented Nov 27, 2024

Fixed: #297

Tasks:

@Gulin7
Copy link
Contributor Author

Gulin7 commented Nov 27, 2024

@Alex-PLACET I had problems with the set-up and I couldn't test locally, but I'll gladly test if you could give me a hand :)

@@ -101,6 +102,17 @@ target_link_libraries(${test_target}
include(doctest)
doctest_discover_tests(${test_target})

set(test_target_catch2 "test_sparrow_lib_catch2")

add_executable(${test_target_catch2} ${SPARROW_TESTS_SOURCES})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not use SPARROW_TESTS_SOURCES but only test_algorithm.cpp

DEPENDS ${test_target}
COMMENT "Running tests with JUnit report saved to: ${JUNIT_REPORT_FILE}"
COMMAND ${test_target} --reporters=better_junit --out=${JUNIT_REPORT_FILE_DOCTEST} --no-path-filenames=true
COMMAND ${test_target_catch2} --reporters=better_junit --out=${JUNIT_REPORT_FILE_CATCH2} --no-path-filenames=true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not use the same options as doctest, please check: https://github.com/catchorg/Catch2/blob/devel/docs/command-line.md

@Alex-PLACET
Copy link
Collaborator

@Alex-PLACET I had problems with the set-up and I couldn't test locally, but I'll gladly test if you could give me a hand :)

Sure, how can I help you ? Where is your issue ?

@Alex-PLACET
Copy link
Collaborator

btw, I approved the run on the CI: https://github.com/man-group/sparrow/pull/301/checks

@Gulin7
Copy link
Contributor Author

Gulin7 commented Nov 27, 2024

There is something wrong with how I've set things up (it's my first time working directly with CMakes and vcpkg and so on).
When I build, I get this error: "test_algorithm.obj : error LNK2019: unresolved external symbol ..." multiple times.
image

@Alex-PLACET
Copy link
Collaborator

@Gulin7 According to the documentation, you need to link the Catch2::Catch2WithMain cmake target and not Catch2::Catch2

@Gulin7
Copy link
Contributor Author

Gulin7 commented Nov 27, 2024

@Alex-PLACET I've tried Catch2::Catch2WithMain and I get the same error as before

@Alex-PLACET
Copy link
Collaborator

sparrow-17-13-20.patch
With this patch, it compiles

@Gulin7
Copy link
Contributor Author

Gulin7 commented Nov 27, 2024

Thank you, the build worked locally! :) I've ran both the tests with Catch2 and the ones with Doctest and they were successful.

@Alex-PLACET
Copy link
Collaborator

To fix the issue with MSVC compilation:
sparrow-09-22-45.patch
Next thing is the issue with the CI: https://github.com/man-group/sparrow/actions/runs/12055549842/job/33616129801?pr=301
Check the annotation at the top.
And I guess it will be ready after the fixes

- howardhinnant_date
# Documentation
- doxygen
- graphviz
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what is the issue but can you revert the indent be sure that you have blank line ?
https://github.com/man-group/sparrow/actions/runs/12066345620/job/33646992095?pr=301

@Alex-PLACET
Copy link
Collaborator

To fix the compilation on Windows with VCPKG, you have to force the build type to release:
next to https://github.com/man-group/sparrow/blob/main/.github/workflows/windows.yml#L128
echo "VCPKG_BUILD_TYPE='Release'" >> $GITHUB_ENV

@Alex-PLACET
Copy link
Collaborator

On exotic architectures, we get an old version of catch2 (version 2.13):
https://github.com/man-group/sparrow/actions/runs/12066345591/job/33646983709?pr=301#step:5:137
There is no cmake target Catch2::Catch2WithMain available with this version
In that case we need #define CATCH_CONFIG_MAIN
https://github.com/catchorg/Catch2/blob/v2.x/docs/tutorial.md#scaling-up
You have to create a main.cpp file which will be used only when we get Catch2 version < 3

@Gulin7
Copy link
Contributor Author

Gulin7 commented Nov 28, 2024

Isn't there a way to enforce Catch2 >= 3.0.0?

@Alex-PLACET
Copy link
Collaborator

I will try to update exotic architectures images to ubuntu 22.04

@Gulin7
Copy link
Contributor Author

Gulin7 commented Nov 28, 2024

Ok, hopefully that fixes it. I've added echo "VCPKG_BUILD_TYPE='Release'" >> $GITHUB_ENV to force the build type to release.

@Alex-PLACET
Copy link
Collaborator

Hmmm, finally forcing VCPKG is not the clean way.
Conda don't provide dependencies in debug.
We should pass a define to CMAKE to force the use of the RuntimeLibrary in Debug or Release: https://cmake.org/cmake/help/latest/prop_tgt/MSVC_RUNTIME_LIBRARY.html when we use Conda

@Gulin7
Copy link
Contributor Author

Gulin7 commented Nov 28, 2024

Would adding these inside if(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
target_compile_options(${test_target} PRIVATE $&lt;$CONFIG:Debug:/MDd> $<$CONFIG:Release:/MD>)
target_compile_options(${test_target_catch2} PRIVATE $&lt;$CONFIG:Debug:/MDd> $<$CONFIG:Release:/MD>)
work?

@Alex-PLACET
Copy link
Collaborator

I think we should try to pass to -DMSVC_RUNTIME_LIBRARY=MultiThreadedDLL when on Windows we use Conda.
It means that we can remove the latest modification about to set MD and _ITERATOR_DEBUG_LEVEL, and in the windows ci matrix: https://github.com/man-group/sparrow/blob/main/.github/workflows/windows.yml#L116
replace
echo "SPARROW_ADDITIONAL_OPTIONS=-DSPARROW_TARGET_32BIT=ON" >> $GITHUB_ENV
by
echo "SPARROW_ADDITIONAL_OPTIONS=-DSPARROW_TARGET_32BIT=ON -DMSVC_RUNTIME_LIBRARY=MultiThreadedDLL" >> $GITHUB_ENV
I'm not sure it's sufficient to handle _ITERATOR_DEBUG_LEVEL
On my side I'm close to use Ubuntu 22 and a recent version of catch2 on exotic architectures


- name: Set dependencies install prefix dir for 32bit
if: matrix.target-arch.name == 'Win32'
run: |
echo "SPARROW_DEPS_PREFIX=$VCPKG_INSTALLED_DIR" >> $GITHUB_ENV
echo "SPARROW_INSTALL_PREFIX=$VCPKG_INSTALLED_DIR" >> $GITHUB_ENV
echo "VCPKG_TARGET_TRIPLET='x86-windows'" >> $GITHUB_ENV
echo "VCPKG_BUILD_TYPE='Release'" >> $GITHUB_ENV
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To remove

@Alex-PLACET
Copy link
Collaborator

According to the builds, it seems that we have to define _ITERATOR_DEBUG_LEVEL=0 when MSVC_RUNTIME_LIBRARY == MultiThreadedDLL
So in CMake, check that MSVC_RUNTIME_LIBRARY == MultiThreadedDLL and apply the _ITERATOR_DEBUG_LEVEL=0 definition to the targets.
We are very close to the end of this issue 🤞

@Alex-PLACET
Copy link
Collaborator

To avoid the issues of Catch2 version on exotics architecture, can you introduce this following mechanism:

set(DOWNLOAD_TEST_DEPENDENCIES ON CACHE BOOL "Download test dependencies")
if(DOWNLOAD_TEST_DEPENDENCIES)
    Include(FetchContent)
    message(STATUS "Downloading doctest")
    FetchContent_Declare(doctest GIT_REPOSITORY https://github.com/onqtam/doctest       GIT_TAG v2.4.11)
    message(STATUS "Downloading catch2")
    FetchContent_Declare(catch2  GIT_REPOSITORY https://github.com/catchorg/Catch2.git  GIT_TAG v3.7.1)
    FetchContent_MakeAvailable(doctest catch2)
else()
    find_package(doctest 2 REQUIRED)
    find_package(catch2 3 REQUIRED)
endif()

@Gulin7
Copy link
Contributor Author

Gulin7 commented Nov 30, 2024

Yes, I'll add both changes on Monday.

@Alex-PLACET
Copy link
Collaborator

@Gulin7 After some internal discussion, let's drop the CMake fetch solution.
Can you modify the the exotics arch CI to pull, compile and install the right Catch2 version ?

@Alex-PLACET Alex-PLACET marked this pull request as draft December 3, 2024 10:47
@Alex-PLACET
Copy link
Collaborator

Alex-PLACET commented Dec 9, 2024

@Gulin7 Can you add me as collaborator on your branch ? I would like to push some commits

@Alex-PLACET Alex-PLACET force-pushed the move-tests-from-doctest-to-catch2 branch from 6784821 to f2b4cc6 Compare December 10, 2024 13:35
@Alex-PLACET Alex-PLACET force-pushed the move-tests-from-doctest-to-catch2 branch from 31a341d to 2683b29 Compare December 17, 2024 15:05
@Alex-PLACET Alex-PLACET merged commit 4e343a0 into man-group:main Dec 18, 2024
71 of 72 checks passed
@Alex-PLACET
Copy link
Collaborator

@Gulin7 Thank you for your work!
The issue was more difficult than expected because of some build system and CI problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move tests from doctest to Catch2
3 participants